Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ZIP-based template support with Pebble rendering, dynamic discovery/loading of template archives from Environment.TEMPLATES_DIR, template API extensions (templateNameStr, thumbData, nullable language/SDKs), asset packaging/installers for a core template archive, and UI thumbnail/name updates with logging. Changes
Sequence DiagramsequenceDiagram
participant Client
participant TemplateProviderImpl
participant ZipTemplateReader
participant ZipFile
participant ZipRecipeExecutor
participant PebbleEngine
participant FileSystem
Client->>TemplateProviderImpl: getTemplates()
TemplateProviderImpl->>FileSystem: list files in Environment.TEMPLATES_DIR
loop per archive
TemplateProviderImpl->>ZipTemplateReader: read(zipFile)
ZipTemplateReader->>ZipFile: open archive & read templates.json
loop per template ref
ZipTemplateReader->>ZipFile: read template.json + thumb.png
ZipTemplateReader->>TemplateProviderImpl: construct ProjectTemplate (baseZipProject)
end
ZipTemplateReader->>ZipFile: close
end
Client->>ZipRecipeExecutor: execute(executor) (when creating project)
ZipRecipeExecutor->>ZipFile: open archive
ZipRecipeExecutor->>PebbleEngine: render .peb templates with params
loop per entry
ZipRecipeExecutor->>ZipRecipeExecutor: shouldSkipFile(language)?
alt peb template
PebbleEngine-->>ZipRecipeExecutor: rendered content
else raw file
ZipRecipeExecutor-->>ZipRecipeExecutor: copy bytes
end
ZipRecipeExecutor->>FileSystem: write file to project dir
end
ZipRecipeExecutor->>ZipFile: close
ZipRecipeExecutor->>Client: return ProjectTemplateRecipeResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (6)
templates-impl/src/main/java/com/itsaky/androidide/templates/impl/TemplateProviderImpl.kt (2)
54-54: Remove commented-out code.The commented line
// val folder = File("/sdcard/Download/templates")should be removed before merging.♻️ Cleanup
private fun initializeTemplates() { - // val folder = File("/sdcard/Download/templates") val folder = TEMPLATES_DIR🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates-impl/src/main/java/com/itsaky/androidide/templates/impl/TemplateProviderImpl.kt` at line 54, Remove the leftover commented-out declaration in TemplateProviderImpl.kt: delete the line "// val folder = File("/sdcard/Download/templates")" (the unused commented variable `folder`) so the class TemplateProviderImpl no longer contains stray commented code before merging.
90-95: Remove commented-out coroutine code or implement async loading.The commented-out
CoroutineScope(Dispatchers.IO).launchsuggests async loading was considered. Either implement it or remove the commented code. Note thatinitializeTemplates()currently runs synchronously, which could block if called on the main thread.♻️ Cleanup commented code
override fun reload() { release() - // CoroutineScope(Dispatchers.IO).launch { initializeTemplates() - // } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates-impl/src/main/java/com/itsaky/androidide/templates/impl/TemplateProviderImpl.kt` around lines 90 - 95, The reload() method in TemplateProviderImpl contains commented-out coroutine code around initializeTemplates(), so either remove the commented CoroutineScope(Dispatchers.IO).launch lines to clean up synchronous behavior or actually perform async loading by running initializeTemplates() inside a CoroutineScope(Dispatchers.IO).launch (or a passed-in scope), ensuring you keep release() semantics, catch and log exceptions from initializeTemplates(), and marshal any UI/thread-bound callbacks back to the main dispatcher; refer to TemplateProviderImpl.reload(), initializeTemplates(), CoroutineScope(Dispatchers.IO).launch and Dispatchers.IO when making the change.templates-api/src/main/java/com/itsaky/androidide/templates/base/ProjectTemplateBuilder.kt (1)
194-204: Non-null assertions follow existing pattern but carry crash risk.The
!!operators ontemplateNameStrandthumbDataare consistent with the existing pattern fortemplateName!!,thumb!!,widgets!!, andrecipe!!. However, this means if a caller fails to set these properties before callingbuildInternal(), aNullPointerExceptionwill be thrown at runtime.Consider whether more graceful error handling (e.g.,
checkNotNull()with a descriptive message) would improve debuggability, consistent with howdefModuleis validated at line 56.♻️ Optional improvement for better error messages
override fun buildInternal(): ProjectTemplate = ProjectTemplate( modules, - templateName!!, - thumb!!, + checkNotNull(templateName) { "templateName must be set before building" }, + checkNotNull(thumb) { "thumb must be set before building" }, tooltipTag, - widgets!!, - recipe!!, - templateNameStr!!, - thumbData!! + checkNotNull(widgets) { "widgets must be set before building" }, + checkNotNull(recipe) { "recipe must be set before building" }, + checkNotNull(templateNameStr) { "templateNameStr must be set before building" }, + checkNotNull(thumbData) { "thumbData must be set before building" } )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates-api/src/main/java/com/itsaky/androidide/templates/base/ProjectTemplateBuilder.kt` around lines 194 - 204, Replace direct non-null assertions in ProjectTemplateBuilder.buildInternal() with explicit checks using checkNotNull (or requireNotNull) that include descriptive messages; specifically validate templateNameStr and thumbData (and optionally templateName, thumb, widgets, recipe) before constructing ProjectTemplate so failures produce clear error messages similar to the defModule validation pattern, referencing buildInternal(), ProjectTemplateBuilder, and the properties templateNameStr and thumbData to locate where to add the checks.app/src/main/java/com/itsaky/androidide/adapters/TemplateListAdapter.kt (1)
78-82: Remove or reduce verbose debug logging before merging.The debug logs at lines 78-80 and 82 are useful during development but may be excessive for production. They're called for every item in the RecyclerView, potentially logging large amounts of data.
♻️ Suggested cleanup
- log.debug("template: $template") templateName.text = template.templateNameStr - log.debug("text: ${template.templateNameStr} templateName.text: ${templateName.text}") if (template.thumbData != null) { - log.debug("thumbData is not null") Glide.with(templateIcon.context) .asBitmap() .load(template.thumbData) .into(templateIcon) } else { templateIcon.setImageResource(template.thumb) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/adapters/TemplateListAdapter.kt` around lines 78 - 82, In TemplateListAdapter (inside the RecyclerView binding code, e.g., onBindViewHolder or bind method) remove or reduce the three per-item debug statements (the log.debug calls that print "template: $template", "text: ${template.templateNameStr} ..." and "thumbData is not null"); either delete them for production or wrap them behind a debug-level guard (e.g., log.isDebugEnabled) or replace with a single lightweight log that avoids printing full objects, and leave a single conditional debug for thumbData if absolutely needed.templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipTemplateReader.kt (2)
60-64: Remove commented-out code before merging.This commented block appears to be dead code. If the widget parsing logic is needed for future work, consider tracking it in an issue rather than leaving commented code in the codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipTemplateReader.kt` around lines 60 - 64, Remove the commented-out widget parsing block to clean up dead code: delete the commented lines that define the widgets variable alternatives (the lines referencing widgetsEntry, zip.getInputStream, parseWidgetsFromJson(...) and the fallback metaJson.widgets.mapNotNull { it.toWidget() }) so only the active widget-parsing implementation remains; if this alternative logic must be preserved for future reference, create an issue or add a brief note in the commit message instead of leaving commented code in ZipTemplateReader.kt.
86-92: Prefer narrow exception handling over broadcatch (e: Exception).Using a broad
catch (e: Exception)masks specific failure modes and can hide bugs. Per project learnings, catch only the specific exception types expected (e.g.,IOException,JsonSyntaxException) and let unexpected exceptions propagate for fail-fast behavior during development.♻️ Proposed narrower exception handling
- } catch (e: Exception) { + } catch (e: IOException) { + log.error("Failed to load template at ${templateRef.path}", e) + } catch (e: com.google.gson.JsonSyntaxException) { log.error("Failed to load template at ${templateRef.path}", e) } } - } catch (e: Exception) { + } catch (e: IOException) { + log.error("Failed to read zip file $zipFile", e) + } catch (e: java.util.zip.ZipException) { log.error("Failed to read zip file $zipFile", e) - // return emptyList() }Based on learnings: "prefer narrow exception handling that catches only the specific exception type reported in crashes (such as IllegalArgumentException) instead of a broad catch-all."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipTemplateReader.kt` around lines 86 - 92, Replace the broad catch blocks in ZipTemplateReader with narrow, specific exception handlers: for the inner block that logs "Failed to load template at ${templateRef.path}" catch expected parsing/validation exceptions (e.g., JsonSyntaxException, IllegalArgumentException) and IO-related exceptions separately and log them; for the outer block that logs "Failed to read zip file $zipFile" catch IOExceptions (and ZipException if applicable) only and let other unexpected exceptions propagate. Update the catch clauses around the code that reads/parses entries (references: ZipTemplateReader, the inner template-loading try block and the outer zip-reading try block) to use multiple specific catches rather than catch (e: Exception).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/src/main/java/com/itsaky/androidide/fragments/TemplateDetailsFragment.kt`:
- Around line 139-142: Replace the hardcoded "unknown" fallback with a string
resource: add <string name="language_unknown">Unknown</string> to strings.xml
and update the assignment in TemplateDetailsFragment (where language is set from
result.data.language?.name) to use getString(R.string.language_unknown) (or
requireContext().getString(...)) as the default; this preserves i18n and keeps
the nullable-safe fallback intact.
In
`@composite-builds/build-deps-common/constants/src/main/java/org/adfa/constants/constants.kt`:
- Around line 81-82: The constant TEMPLATE_CORE_ARCHIVE is incorrectly
concatenated as "core$TEMPLATE_ARCHIVE_EXTENSION" which yields "corecgt" instead
of "core.cgt"; update the concatenation for TEMPLATE_CORE_ARCHIVE to include the
missing dot (e.g., "core.$TEMPLATE_ARCHIVE_EXTENSION" or equivalent) so it
matches File.extension behavior and produces "core.cgt".
In `@templates-api/src/main/java/com/itsaky/androidide/templates/template.kt`:
- Around line 344-348: The new requireNotNull(templateNameStr) in
Template<R>.build() causes templates constructed via
baseProject()/baseProjectImpl() (e.g., emptyActivityProject(),
noActivityProjectTemplate()) to throw because they never set templateNameStr;
either remove/relax the require in build() so templateNameStr is optional at
build time, or update all baseProject*/template builders to set templateNameStr
before calling build(); locate the check in build() (template.kt) and choose one
fix: (A) delete or replace requireNotNull(templateNameStr) with an optional
fallback/nullable handling, or (B) ensure baseProject(), baseProjectImpl(),
emptyActivityProject(), noActivityProjectTemplate() set templateNameStr (similar
to ZipTemplateReader which assigns this.templateNameStr = metaJson.name) prior
to build().
In
`@templates-impl/src/main/java/com/itsaky/androidide/templates/impl/TemplateProviderImpl.kt`:
- Around line 76-78: Replace the broad catch (e: Throwable) {
e.printStackTrace() } in TemplateProviderImpl with a narrow catch for the
specific exception(s) observed in crashes (e.g., catch (e:
IllegalArgumentException)) and use the class SLF4J logger to record the error
(e.g., logger.error("Descriptive message about what failed", e)); if multiple
specific exceptions can occur, add separate catches for them rather than
catching Throwable or Exception.
In
`@templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipJson.kt`:
- Around line 84-107: The toWidget() implementation currently calls
StringParameterBuilder.build() and BooleanParameterBuilder.build() without
providing required fields (name and for string default), causing NPEs; guard
these calls by validating WidgetJson.name (and WidgetJson.default for "string")
before constructing builders—if name is null/blank or string default is null,
log a warning and return null instead of building; update the "string" and
"boolean" branches in toWidget() to perform these checks prior to using
StringParameterBuilder and BooleanParameterBuilder so build() is never called
with missing required values.
In
`@templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt`:
- Around line 44-48: The ZipFile returned by zipProvider() (variable zip in
ZipRecipeExecutor.kt) is never closed; wrap the zipProvider() call so the
ZipFile is closed after use (e.g., use Kotlin's .use { zip -> ... } or a
try/finally that calls zip.close()), moving the early-return logic that returns
ProjectTemplateRecipeResultImpl(data) inside the resource block so the zip is
closed in all code paths before returning.
- Around line 93-98: Pebble template parsing/evaluation in the ZipRecipeExecutor
(calls to pebbleEngine.getTemplate(content) and template.evaluate(writer,
params) while processing entry.name) can throw PebbleException and currently
will abort ZIP processing; wrap the template retrieval and evaluation for
TEMPLATE_EXTENSION entries in a try/catch that catches PebbleException (and
optionally Exception) so a single bad template is logged via log.error including
entry.name and the exception, and then skip writing the outFile for that entry
(or write a safe fallback) and continue processing the rest of the ZIP; ensure
you still close any writers/streams (StringWriter) in finally or use Kotlin
use/try-with-resources patterns so resources are not leaked.
- Around line 89-103: The current code reads the entry content once into content
via zip.getInputStream(entry).bufferedReader().readText() but then re-opens
streams and creates a FileOutputStream even for template files
(TEMPLATE_EXTENSION) where outFile.writeText(...) is used instead of the opened
output, causing redundant IO and brief file locks; fix by branching on
entry.name.endsWith(TEMPLATE_EXTENSION) before opening the input/output streams:
for template files use the already-read content with
pebbleEngine.getTemplate(content) / template.evaluate(...) and write via
outFile.writeText(...); for non-template files skip reading into content and
instead open zip.getInputStream(entry).use { input ->
FileOutputStream(outFile).use { output -> input.copyTo(output) } } so
FileOutputStream is only created when actually used and streams are not
duplicated.
In
`@templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipTemplateReader.kt`:
- Around line 29-31: ZipFile opened in ZipTemplateReader (the local variable
zip) is never closed; wrap ZipFile(zipFile) in a Kotlin use { } block or
otherwise ensure zip.close() is called (e.g., try-with-resources style) inside
the method that constructs it so the ZipFile is closed on all code paths and
avoids leaking file handles.
---
Nitpick comments:
In `@app/src/main/java/com/itsaky/androidide/adapters/TemplateListAdapter.kt`:
- Around line 78-82: In TemplateListAdapter (inside the RecyclerView binding
code, e.g., onBindViewHolder or bind method) remove or reduce the three per-item
debug statements (the log.debug calls that print "template: $template", "text:
${template.templateNameStr} ..." and "thumbData is not null"); either delete
them for production or wrap them behind a debug-level guard (e.g.,
log.isDebugEnabled) or replace with a single lightweight log that avoids
printing full objects, and leave a single conditional debug for thumbData if
absolutely needed.
In
`@templates-api/src/main/java/com/itsaky/androidide/templates/base/ProjectTemplateBuilder.kt`:
- Around line 194-204: Replace direct non-null assertions in
ProjectTemplateBuilder.buildInternal() with explicit checks using checkNotNull
(or requireNotNull) that include descriptive messages; specifically validate
templateNameStr and thumbData (and optionally templateName, thumb, widgets,
recipe) before constructing ProjectTemplate so failures produce clear error
messages similar to the defModule validation pattern, referencing
buildInternal(), ProjectTemplateBuilder, and the properties templateNameStr and
thumbData to locate where to add the checks.
In
`@templates-impl/src/main/java/com/itsaky/androidide/templates/impl/TemplateProviderImpl.kt`:
- Line 54: Remove the leftover commented-out declaration in
TemplateProviderImpl.kt: delete the line "// val folder =
File("/sdcard/Download/templates")" (the unused commented variable `folder`) so
the class TemplateProviderImpl no longer contains stray commented code before
merging.
- Around line 90-95: The reload() method in TemplateProviderImpl contains
commented-out coroutine code around initializeTemplates(), so either remove the
commented CoroutineScope(Dispatchers.IO).launch lines to clean up synchronous
behavior or actually perform async loading by running initializeTemplates()
inside a CoroutineScope(Dispatchers.IO).launch (or a passed-in scope), ensuring
you keep release() semantics, catch and log exceptions from
initializeTemplates(), and marshal any UI/thread-bound callbacks back to the
main dispatcher; refer to TemplateProviderImpl.reload(), initializeTemplates(),
CoroutineScope(Dispatchers.IO).launch and Dispatchers.IO when making the change.
In
`@templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipTemplateReader.kt`:
- Around line 60-64: Remove the commented-out widget parsing block to clean up
dead code: delete the commented lines that define the widgets variable
alternatives (the lines referencing widgetsEntry, zip.getInputStream,
parseWidgetsFromJson(...) and the fallback metaJson.widgets.mapNotNull {
it.toWidget() }) so only the active widget-parsing implementation remains; if
this alternative logic must be preserved for future reference, create an issue
or add a brief note in the commit message instead of leaving commented code in
ZipTemplateReader.kt.
- Around line 86-92: Replace the broad catch blocks in ZipTemplateReader with
narrow, specific exception handlers: for the inner block that logs "Failed to
load template at ${templateRef.path}" catch expected parsing/validation
exceptions (e.g., JsonSyntaxException, IllegalArgumentException) and IO-related
exceptions separately and log them; for the outer block that logs "Failed to
read zip file $zipFile" catch IOExceptions (and ZipException if applicable) only
and let other unexpected exceptions propagate. Update the catch clauses around
the code that reads/parses entries (references: ZipTemplateReader, the inner
template-loading try block and the outer zip-reading try block) to use multiple
specific catches rather than catch (e: Exception).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1c8437c1-522d-4e39-b8ee-cf69e7019cb9
📒 Files selected for processing (16)
app/src/main/java/com/itsaky/androidide/adapters/TemplateListAdapter.ktapp/src/main/java/com/itsaky/androidide/fragments/TemplateDetailsFragment.ktapp/src/main/java/com/itsaky/androidide/fragments/TemplateListFragment.ktcommon/src/main/java/com/itsaky/androidide/utils/Environment.javacomposite-builds/build-deps-common/constants/src/main/java/org/adfa/constants/constants.ktgradle/libs.versions.tomltemplates-api/build.gradle.ktstemplates-api/src/main/java/com/itsaky/androidide/templates/base/ProjectTemplateBuilder.kttemplates-api/src/main/java/com/itsaky/androidide/templates/base/base.kttemplates-api/src/main/java/com/itsaky/androidide/templates/template.kttemplates-impl/build.gradle.ktstemplates-impl/src/main/java/com/itsaky/androidide/templates/impl/TemplateProviderImpl.kttemplates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipJson.kttemplates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kttemplates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipTemplateConstants.kttemplates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipTemplateReader.kt
app/src/main/java/com/itsaky/androidide/fragments/TemplateDetailsFragment.kt
Outdated
Show resolved
Hide resolved
composite-builds/build-deps-common/constants/src/main/java/org/adfa/constants/constants.kt
Outdated
Show resolved
Hide resolved
templates-impl/src/main/java/com/itsaky/androidide/templates/impl/TemplateProviderImpl.kt
Outdated
Show resolved
Hide resolved
templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipJson.kt
Outdated
Show resolved
Hide resolved
templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt
Outdated
Show resolved
Hide resolved
templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt
Outdated
Show resolved
Hide resolved
templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt
Outdated
Show resolved
Hide resolved
templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipTemplateReader.kt
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@templates-api/src/main/java/com/itsaky/androidide/templates/base/modules/android/buildGradle.kt`:
- Line 100: The template currently interpolates data.versions.minSdk?.api
directly (e.g. the string "minSdk = ${data.versions.minSdk?.api}") which can
emit "minSdk = null"; change the template generation so the entire "minSdk =
..." line is only emitted when data.versions.minSdk?.api is non-null (use a
conditional/let to omit the line if null) and apply this fix to all four
occurrences in buildGradle.kt (the KTS and Groovy template generators and their
NDK variants where data.versions.minSdk?.api is used).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 19f0d752-60b8-48c1-a5e7-bdc05c91538b
📒 Files selected for processing (1)
templates-api/src/main/java/com/itsaky/androidide/templates/base/modules/android/buildGradle.kt
templates-api/src/main/java/com/itsaky/androidide/templates/base/modules/android/buildGradle.kt
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt`:
- Around line 83-91: The new TEMPLATE_CORE_ARCHIVE case copies the bundled
archive but BundledAssetsInstaller.expectedSize() still returns 0 for
TEMPLATE_CORE_ARCHIVE, causing progress misreporting; update
BundledAssetsInstaller.expectedSize() to return the real expected size for
TEMPLATE_CORE_ARCHIVE (matching how other assets are handled) by computing or
returning the compressed or uncompressed byte length used by the installer so
AssetsInstallationHelper.totalSize includes TEMPLATE_CORE_ARCHIVE; locate the
expectedSize() method and add a branch for TEMPLATE_CORE_ARCHIVE (referencing
TEMPLATE_CORE_ARCHIVE constant) that returns the correct long value (or derives
it from the same source ToolsManager.getCommonAsset / asset metadata) to ensure
progress reflects the actual copy work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6155d922-1073-4c2e-be78-7f8dfd09e922
📒 Files selected for processing (5)
app/build.gradle.ktsapp/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.ktapp/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.ktapp/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.ktcomposite-builds/build-deps-common/constants/src/main/java/org/adfa/constants/constants.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- composite-builds/build-deps-common/constants/src/main/java/org/adfa/constants/constants.kt
app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt (1)
94-108:⚠️ Potential issue | 🟠 MajorRemove redundant stream work and ensure every opened stream is closed.
Line 94 reads the whole entry (as text) before branching, and that stream is not managed with
use. Also,FileOutputStreamis opened for template entries where it is never used.Proposed fix
- val content = zip.getInputStream(entry).bufferedReader().readText() - - zip.getInputStream(entry).use { input -> - FileOutputStream(outFile).use { output -> - if (entry.name.endsWith(TEMPLATE_EXTENSION)) { - log.debug("template processing ${entry.name}") - val template = pebbleEngine.getTemplate(content) - val writer = StringWriter() - template.evaluate(writer, params) - outFile.writeText(writer.toString(), Charsets.UTF_8) - } else { - input.copyTo(output) - } - } - } + if (entry.name.endsWith(TEMPLATE_EXTENSION)) { + log.debug("template processing ${entry.name}") + val content = zip.getInputStream(entry).bufferedReader().use { it.readText() } + val template = pebbleEngine.getTemplate(content) + val writer = StringWriter() + template.evaluate(writer, params) + outFile.writeText(writer.toString(), Charsets.UTF_8) + } else { + zip.getInputStream(entry).use { input -> + FileOutputStream(outFile).use { output -> + input.copyTo(output) + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt` around lines 94 - 108, The code reads the entry into val content via zip.getInputStream(entry).bufferedReader().readText() without using use(), then later opens zip.getInputStream(entry) again and always creates a FileOutputStream(outFile) even for template entries; fix ZipRecipeExecutor by removing the initial readText call, only open the entry InputStream inside a use block when copying binary entries, and for TEMPLATE_EXTENSION entries read the entry via a buffered reader in a use block (or obtain the entry content via a single use-managed stream) and write the rendered template with outFile.writeText (creating FileOutputStream only when actually copying binary data), ensuring every InputStream/FileOutputStream/Reader/Writer is wrapped with use so all streams are closed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt`:
- Around line 85-93: The code constructs output paths from ZipEntry names
(relativePath, outFile) without validating them, allowing zip-slip path
traversal; fix by resolving the target path against projectDir and verifying the
canonical path is inside projectDir before creating/writing: after computing
outFile (from relativePath.removeSuffix(TEMPLATE_EXTENSION)), call canonicalFile
on both projectDir and outFile and ensure outFile.canonicalPath
startsWith(projectDir.canonicalPath + File.separator) (or equals
projectDir.canonicalPath) and if not, skip or throw; only then create parent
dirs (outFile.parentFile?.mkdirs()) and write the file, keeping the existing
handling for directories (entry.isDirectory) intact and apply the same
validation for directory entries.
---
Duplicate comments:
In
`@templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt`:
- Around line 94-108: The code reads the entry into val content via
zip.getInputStream(entry).bufferedReader().readText() without using use(), then
later opens zip.getInputStream(entry) again and always creates a
FileOutputStream(outFile) even for template entries; fix ZipRecipeExecutor by
removing the initial readText call, only open the entry InputStream inside a use
block when copying binary entries, and for TEMPLATE_EXTENSION entries read the
entry via a buffered reader in a use block (or obtain the entry content via a
single use-managed stream) and write the rendered template with
outFile.writeText (creating FileOutputStream only when actually copying binary
data), ensuring every InputStream/FileOutputStream/Reader/Writer is wrapped with
use so all streams are closed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4713170a-32e7-4bb4-af9f-466c0a3bb638
📒 Files selected for processing (4)
templates-impl/src/main/java/com/itsaky/androidide/templates/impl/TemplateProviderImpl.kttemplates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipJson.kttemplates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kttemplates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipTemplateReader.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipTemplateReader.kt
templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt (1)
150-154: Consider making helper methods private to reduce API surface.These utilities appear internal to
ZipRecipeExecutorand don’t need to be publicly exposed.Also applies to: 156-159, 226-234
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt` around lines 150 - 154, Make the utility helpers non-public: change safeLanguageName and safeMinSdkApi to private functions (e.g., private fun safeLanguageName(...) and private fun safeMinSdkApi(...)) and likewise change the other helper methods in the referenced ranges (startLine 156-159 and 226-234) to private; ensure you update any internal references within ZipRecipeExecutor to match the new visibility and if any external code depends on them, move those call sites into the class or provide a controlled public facade instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt`:
- Around line 43-48: Check data.projectDir.exists() before calling zipProvider()
so archive IO is not touched when we can short-circuit; move the projectDir
existence check to occur prior to invoking zipProvider() in ZipRecipeExecutor
(returning ProjectTemplateRecipeResultImpl(data) when it exists) to avoid
opening the ZIP unnecessarily.
In
`@templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipTemplateReader.kt`:
- Around line 101-103: In ZipTemplateReader, replace the broad catch blocks that
currently catch Exception with specific exception types: for the inner catch
that logs "Failed to load template at ${templateRef.path}" catch
java.io.IOException and com.google.gson.JsonParseException only (log the error
as before); for the outer catch around the ZIP/index opening logic catch
java.util.zip.ZipException, java.io.IOException and
com.google.gson.JsonParseException only; remove the generic catch so unexpected
exceptions propagate (and add the necessary imports).
---
Nitpick comments:
In
`@templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt`:
- Around line 150-154: Make the utility helpers non-public: change
safeLanguageName and safeMinSdkApi to private functions (e.g., private fun
safeLanguageName(...) and private fun safeMinSdkApi(...)) and likewise change
the other helper methods in the referenced ranges (startLine 156-159 and
226-234) to private; ensure you update any internal references within
ZipRecipeExecutor to match the new visibility and if any external code depends
on them, move those call sites into the class or provide a controlled public
facade instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5d5e865c-daee-4476-9acb-56fe7bdc0781
📒 Files selected for processing (4)
app/build.gradle.ktsapp/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kttemplates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kttemplates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipTemplateReader.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- app/build.gradle.kts
- app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt
templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt
Outdated
Show resolved
Hide resolved
templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipTemplateReader.kt
Outdated
Show resolved
Hide resolved
jomen-adfa
force-pushed
the
feat/ADFA-2737-template-initial
branch
from
07c511d to
001241e
Compare
jomen-adfa
requested review from
Daniel-ADFA,
davidschachterADFA,
hal-eisen-adfa,
itsaky-adfa and
jimturner-adfa
itsaky-adfa
requested changes
Mar 20, 2026
Contributor
itsaky-adfa
left a comment
itsaky-adfa
left a comment
There was a problem hiding this comment.
Please also run Spotless on this branch to format all files appropriately.
app/src/main/java/com/itsaky/androidide/adapters/TemplateListAdapter.kt
Outdated
Show resolved
Hide resolved
templates-api/src/main/java/com/itsaky/androidide/templates/base/modules/android/buildGradle.kt
Show resolved
Hide resolved
| defaultConfig { | ||
| applicationId = "${data.packageName}" | ||
| minSdk = ${data.versions.minSdk.api} | ||
| minSdk = ${data.versions.minSdk?.api} |
Contributor
There was a problem hiding this comment.
Same non-optional input issue.
Contributor
Author
There was a problem hiding this comment.
The new template code is currently co-existing with the legacy template code and these changes were put into the legacy code to defer removal of the legacy template to after the new template code has stabilized. Had to make minSdk optional since there are cases in some templates now, like plugin, and in the future that don't seem to use this in the templates.
Contributor
There was a problem hiding this comment.
If the minSdk is unused, that's fine. Templates can choose to ignore it if required, but I think it should never be undefined.
Although, I wonder why plugin templates don't have minSdk defined (@Daniel-ADFA?). I mean, what happens if a plugin uses APIs from newer Android versions, but the user tries to install that plugin on an older Android version? Wouldn't the plugin crash due to the API differences?
| defaultConfig { | ||
| applicationId = "${data.packageName}" | ||
| minSdk = ${data.versions.minSdk.api} | ||
| minSdk = ${data.versions.minSdk?.api} |
Contributor
There was a problem hiding this comment.
Same non-optional input issue.
|
|
||
| defaultConfig { | ||
| applicationId = "${data.packageName}" | ||
| minSdk = ${data.versions.minSdk.api} |
Contributor
There was a problem hiding this comment.
Same non-optional input issue.
templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipJson.kt
Show resolved
Hide resolved
templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt
Outdated
Show resolved
Hide resolved
templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt
Outdated
Show resolved
Hide resolved
templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt
Outdated
Show resolved
Hide resolved
templates-impl/src/main/java/com/itsaky/androidide/templates/impl/zip/ZipRecipeExecutor.kt
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt`:
- Around line 117-118: The progress size for TEMPLATE_CORE_ARCHIVE is
inconsistent between BundledAssetsInstaller and SplitAssetsInstaller; unify by
computing and using the same expected size source for both installers (e.g.,
derive the decompressed archive size once and expose a shared helper or
constant) and update both installers to call that helper instead of hardcoding
different values; specifically change BundledAssetsInstaller and
SplitAssetsInstaller to reference a single function/constant (e.g.,
getExpectedSize(TEMPLATE_CORE_ARCHIVE) or EXPECTED_SIZES[TEMPLATE_CORE_ARCHIVE])
so expectedSize(TEMPLATE_CORE_ARCHIVE) returns the same value across both paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7b6a3e5-8435-4dc6-9b68-b215b6a61448
📒 Files selected for processing (9)
app/build.gradle.ktsapp/src/main/java/com/itsaky/androidide/adapters/TemplateListAdapter.ktapp/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.ktapp/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.ktapp/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.ktapp/src/main/java/com/itsaky/androidide/fragments/TemplateDetailsFragment.ktcommon/src/main/java/com/itsaky/androidide/utils/Environment.javacomposite-builds/build-deps-common/constants/src/main/java/org/adfa/constants/constants.ktgradle/libs.versions.toml
✅ Files skipped from review due to trivial changes (1)
- gradle/libs.versions.toml
🚧 Files skipped from review as they are similar to previous changes (5)
- common/src/main/java/com/itsaky/androidide/utils/Environment.java
- app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt
- composite-builds/build-deps-common/constants/src/main/java/org/adfa/constants/constants.kt
- app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt
- app/src/main/java/com/itsaky/androidide/adapters/TemplateListAdapter.kt
app/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.kt
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/src/main/java/com/itsaky/androidide/fragments/TemplateDetailsFragment.kt (1)
139-140:⚠️ Potential issue | 🟡 MinorReplace hardcoded fallback text with a localized resource.
Line 140 still uses
"unknown", which bypasses i18n.💡 Proposed fix
- language = result.data.language?.name ?: "unknown" + language = result.data.language?.name ?: getString(R.string.language_unknown)<string name="language_unknown">Unknown</string>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/fragments/TemplateDetailsFragment.kt` around lines 139 - 140, The code in TemplateDetailsFragment sets language = result.data.language?.name ?: "unknown" which hardcodes fallback text; replace the literal "unknown" with a localized string resource (e.g., use context.getString(R.string.language_unknown) or requireContext().getString(...)) and add the corresponding <string name="language_unknown">Unknown</string> to strings.xml so the fallback is localizable; update the assignment that uses template.templateNameStr and result.data.language?.name to use the localized resource when the language is null.
🧹 Nitpick comments (1)
app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt (1)
76-81: Create the templates directory defensively before writingcore.cgt.This avoids a potential
FileNotFoundExceptionif directory initialization order changes.Suggested patch
TEMPLATE_CORE_ARCHIVE -> { - val coreCgt = Environment.TEMPLATES_DIR.resolve(TEMPLATE_CORE_ARCHIVE) + Files.createDirectories(Environment.TEMPLATES_DIR.toPath()) + val coreCgt = Environment.TEMPLATES_DIR.resolve(TEMPLATE_CORE_ARCHIVE) coreCgt.outputStream().use { output -> zipInput.copyTo(output) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt` around lines 76 - 81, The code writes TEMPLATE_CORE_ARCHIVE into Environment.TEMPLATES_DIR without ensuring that directory exists, which can throw FileNotFoundException; before resolving TEMPLATE_CORE_ARCHIVE and opening coreCgt.outputStream() in the SplitAssetsInstaller.kt block handling TEMPLATE_CORE_ARCHIVE, create the directory defensively (e.g., call mkdirs()/createDirectories on Environment.TEMPLATES_DIR) so the parent folder exists before writing the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@app/src/main/java/com/itsaky/androidide/fragments/TemplateDetailsFragment.kt`:
- Around line 139-140: The code in TemplateDetailsFragment sets language =
result.data.language?.name ?: "unknown" which hardcodes fallback text; replace
the literal "unknown" with a localized string resource (e.g., use
context.getString(R.string.language_unknown) or requireContext().getString(...))
and add the corresponding <string name="language_unknown">Unknown</string> to
strings.xml so the fallback is localizable; update the assignment that uses
template.templateNameStr and result.data.language?.name to use the localized
resource when the language is null.
---
Nitpick comments:
In `@app/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.kt`:
- Around line 76-81: The code writes TEMPLATE_CORE_ARCHIVE into
Environment.TEMPLATES_DIR without ensuring that directory exists, which can
throw FileNotFoundException; before resolving TEMPLATE_CORE_ARCHIVE and opening
coreCgt.outputStream() in the SplitAssetsInstaller.kt block handling
TEMPLATE_CORE_ARCHIVE, create the directory defensively (e.g., call
mkdirs()/createDirectories on Environment.TEMPLATES_DIR) so the parent folder
exists before writing the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91e2711f-f38c-493c-831b-3e725aced278
📒 Files selected for processing (9)
app/build.gradle.ktsapp/src/main/java/com/itsaky/androidide/adapters/TemplateListAdapter.ktapp/src/main/java/com/itsaky/androidide/assets/AssetsInstallationHelper.ktapp/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.ktapp/src/main/java/com/itsaky/androidide/assets/SplitAssetsInstaller.ktapp/src/main/java/com/itsaky/androidide/fragments/TemplateDetailsFragment.ktcommon/src/main/java/com/itsaky/androidide/utils/Environment.javacomposite-builds/build-deps-common/constants/src/main/java/org/adfa/constants/constants.ktgradle/libs.versions.toml
🚧 Files skipped from review as they are similar to previous changes (5)
- common/src/main/java/com/itsaky/androidide/utils/Environment.java
- composite-builds/build-deps-common/constants/src/main/java/org/adfa/constants/constants.kt
- gradle/libs.versions.toml
- app/src/main/java/com/itsaky/androidide/adapters/TemplateListAdapter.kt
- app/src/main/java/com/itsaky/androidide/assets/BundledAssetsInstaller.kt
Collaborator
|
I'm not qualified to renew the code. |
alome007
approved these changes
Mar 23, 2026
Daniel-ADFA
approved these changes
Mar 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
initial next gen template